Skip to content

Restrict paired browsing to when dev mode is enabled - #3863

Merged
westonruter merged 3 commits into
developfrom
fix/paired-browsing-requiring-dev-mode
Dec 1, 2019
Merged

Restrict paired browsing to when dev mode is enabled#3863
westonruter merged 3 commits into
developfrom
fix/paired-browsing-requiring-dev-mode

Conversation

@westonruter

Copy link
Copy Markdown
Member

Summary

See https://github.com/ampproject/amp-wp/pull/3656/files#r352310999:

I just realized that this makes every page invalid AMP, because every page is forced to be in dev mode and the custom script is enqueued. Two options I see:

  1. Short-circuit this function if an user is not logged-in (or rather, if amp_is_dev_mode() returns false). Since search crawlers will never be logged-in, they will never then see an invalid AMP page. This would also require entire paired browsing app should wp_die() if the user is not logged in.
  2. Go back to only including this JS if the admin bar is showing. Accessing paired browsing here would still need to include the wp_die() if ! amp_is_dev_mode().

This PR implements the former. This allows a site to continue to allow paired browsing for unauthenticated users by adding plugin code that does add_filter( 'amp_dev_mode_enabled', '__return_true' );, though naturally they'd want to use some other condition (e.g. setting a cookie).

See #3365. Amends #3656.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added this to the v1.5 milestone Dec 1, 2019
@westonruter
westonruter requested a review from pierlon December 1, 2019 00:12
@googlebot googlebot added the cla: yes Signed the Google CLA label Dec 1, 2019
@westonruter westonruter mentioned this pull request Dec 1, 2019
10 tasks

@pierlon pierlon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch @westonruter! I agree with your first option of short-circuiting it if amp_is_dev_mode() returns false.

@pierlon

pierlon commented Dec 1, 2019

Copy link
Copy Markdown
Contributor

While reviewing this I realized that the dialog text does not match the options that are shown if the user can't exit the page:

image

I see two options:

  1. Remove the second sentence
  2. Only show the second sentence if the 'Exit' button is also shown

@westonruter

Copy link
Copy Markdown
Member Author
  • Remove the second sentence

Good point. I've done this in c10276e. I think it makes sense to remove because the text is redundant as the presence or absence of the buttons say the same thing.

@westonruter
westonruter requested a review from pierlon December 1, 2019 03:26

@pierlon pierlon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes work as expected. Ready to ship :shipit:.

@westonruter
westonruter merged commit 0621cea into develop Dec 1, 2019
@westonruter
westonruter deleted the fix/paired-browsing-requiring-dev-mode branch December 1, 2019 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Signed the Google CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants